Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Components: Fix React Compiler error for 'useScrollRectIntoView' #66498

Merged
merged 3 commits into from
Nov 4, 2024

Conversation

Mamaduka
Copy link
Member

What?

Part of #61788.
Similar to #66492.

PR fixes React Compiler errors for the useScrollRectIntoView hook using Element.scroll instead of the shorthand method.

Testing Instructions

Smoke test Tabs components in Storybook and editors.

@Mamaduka Mamaduka added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Oct 26, 2024
@Mamaduka Mamaduka self-assigned this Oct 26, 2024
@Mamaduka Mamaduka requested review from ciampo and tyxla October 26, 2024 11:46
Copy link

github-actions bot commented Oct 26, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@ciampo
Copy link
Contributor

ciampo commented Oct 28, 2024

@Mamaduka unit tests are failing because jsdom doesn't implement scroll-related methods (see jsdom/jsdom#1422 and jsdom/jsdom#2751) — I suspect this was the original cause that pushed Dani to use scrollLeft instead of scroll().

We could either look into adding global mocks for the whole Gutengerg project, or potentially add extra checks (or optional chaining) in the source code and add a comment explaining the reason for it.

@tyxla
Copy link
Member

tyxla commented Oct 28, 2024

IMHO, those mutations should be allowed by the React Compiler, since all those Element APIs are perfectly valid, and the non-readonly properties should be writable, and not considered violations. Perhaps we should consider raising an issue in the React repo?

@Mamaduka
Copy link
Member Author

IMHO, those mutations should be allowed by the React Compiler.

That would be ideal, but the same issue remains with the refs (#29196). I've got a feeling it's a complex problem to resolve, and using Element methods (and similar) might be the only current solution.

@Mamaduka
Copy link
Member Author

Mamaduka commented Nov 1, 2024

Updated in 925b5f5.

Copy link

github-actions bot commented Nov 1, 2024

Flaky tests detected in 925b5f5.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11624389497
📝 Reported issues:

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @Mamaduka.

Just a minor optional suggestion

} else if ( rightOverflow > 0 ) {
parent.scrollLeft = parentScroll + rightOverflow;
parent.scroll?.( { left: parentScroll + rightOverflow } );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely the comment might make sense to be here too.

I wonder if we can isolate the logic to a single parent?.scroll() call. Then, it will be just a single comment.

scrollLeft = parentScroll + rightOverflow;
}

if ( scrollLeft !== null ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a precaution.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Mamaduka!

@Mamaduka Mamaduka merged commit b992649 into trunk Nov 4, 2024
64 of 65 checks passed
@Mamaduka Mamaduka deleted the fix/tablist-compiler-error branch November 4, 2024 11:56
@github-actions github-actions bot added this to the Gutenberg 19.7 milestone Nov 4, 2024
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
…dPress#66498)

* Components: Fix React Compiler error for 'useScrollRectIntoView'
* Use optional chaining and add inline comment
* Use single scroll call

Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants